Skip to content

HIVE-29302: Wider Tez session open scope should be guarded to prevent local resource leaks #6161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
abstractdog wants to merge 4 commits into
base: master
Choose a base branch
from
Open

HIVE-29302: Wider Tez session open scope should be guarded to prevent local resource leaks #6161

abstractdog wants to merge 4 commits into from
+74 −11

Conversation

@abstractdog
Copy link
Contributor

@abstractdog abstractdog commented Nov 3, 2025 •
edited
Loading

What changes were proposed in this pull request?

Encapsulate a wider code path in error-handling while opening a Tez session.
The point of the patch is nothing more than refactoring code to a separate method:

    try {
      openInternalUnsafe(isAsync, console);
    } catch (Exception e) {
      LOG.info("Failed to open session, deleting scratch dir to prevent resource leak...", e);
      cleanupScratchDir();
      throw e;
    }

Why are the changes needed?

A scratch directory can leak if an exception is thrown after it’s created and resources have been placed into it.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test added.

@abstractdog abstractdog requested a review from ayushtkn November 4, 2025 07:42
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 4, 2025

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@abstractdog
Copy link
Contributor Author

abstractdog commented Nov 4, 2025

Quality Gate Passed Quality Gate passed

Issues 2 New issues 0 Accepted issues

Measures 0 Security Hotspots 0.0% Coverage on New Code 0.0% Duplication on New Code

See analysis details on SonarQube Cloud

not about to handle this:

[Either log this exception and handle it, or rethrow it with some contextual information.](https://sonarcloud.io/project/issues?pullRequest=6161&open=AZpLt4QCPQuC7unSj798&id=apache_hive)

no more useful contextual information can be added here than "hey, I caught this exception, cleaned up, and now re-throwing the session open failure exception"

@Aggarwal-Raghav
Copy link
Contributor

Aggarwal-Raghav commented Nov 10, 2025

Changes makes sense to me, to cleanup scratch directory in case of exception before startSessionAndContainers() in openInternal() method

LGTM +1 (non-binding)

deniskuzZ
deniskuzZ reviewed Dec 8, 2025
View reviewed changes
deniskuzZ
deniskuzZ approved these changes Dec 8, 2025
View reviewed changes
Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, minor comment

deniskuzZ
deniskuzZ reviewed Dec 8, 2025
View reviewed changes
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 9, 2025

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deniskuzZ deniskuzZ deniskuzZ approved these changes

@ayushtkn ayushtkn Awaiting requested review from ayushtkn

Assignees

No one assigned

Labels

tests unstable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@abstractdog @Aggarwal-Raghav @deniskuzZ @asf-ci-hive